CEXT-6160: store and expose Commerce system config during app associa…#460
CEXT-6160: store and expose Commerce system config during app associa…#460vinayrao2000 wants to merge 3 commits into
Conversation
|
| "./runtime": { | ||
| "import": { | ||
| "types": "./dist/es/runtime.d.mts", | ||
| "default": "./dist/es/runtime.mjs" | ||
| }, | ||
| "require": { | ||
| "types": "./dist/cjs/runtime.d.cts", | ||
| "default": "./dist/cjs/runtime.cjs" | ||
| } | ||
| }, |
There was a problem hiding this comment.
I would not export this from /runtime, just from the root entrypoint, so instead of:
import { ... } from "@adobe/aio-commerce-lib-app/runtime"it would be:
import { ... } from "@adobe/aio-commerce-lib-app"| name: packageName, | ||
| package: { parameters: [...kept, ...added] }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Probably such an approach would work. But I thought the idea was to store this configuration by using logic from aio-commerce-lib-config. So it can be retrieved in runtime actions from the configuration.
obarcelonap
left a comment
There was a problem hiding this comment.
Thanks for writing a spec for these changes, I only reviewed this part.
Normally you should first pr only with the spec so you can get feedback and then once approved and merged you can go ahead with the implementation.
Spec workflow is defined here
aio-commerce-sdk/specs/conventions.md
Lines 54 to 66 in 9a7f23e
| **Non-goals:** | ||
|
|
||
| - Storing any Commerce system configuration beyond Base URL and deployment type. | ||
| - Replacing the `AIO_COMMERCE_API_BASE_URL` and `AIO_COMMERCE_API_FLAVOR` package params set |
There was a problem hiding this comment.
Wouldn't be better to use these params names? Otherwise the params resolver won't work and might be a bit confusing when to use what.
For instance, how the dev will decide when to use AIO_COMMERCE_API_BASE_URL vs AIO_COMMERCE_BASE_URL
| import { getCommerceSystemConfig } from "@adobe/aio-commerce-lib-app/runtime"; | ||
|
|
||
| export async function main(params) { | ||
| const config = getCommerceSystemConfig(params); |
There was a problem hiding this comment.
While this kind of helper function is needed, since the main purpose of these values is to be able to do http requests to the commerce instance, I think we need also to use it in some way in http client param resolution
aio-commerce-sdk/packages/aio-commerce-lib-api/source/lib/commerce/helpers.ts
Lines 236 to 244 in 9a7f23e
There was a problem hiding this comment.
But then we would be coupling lib-app to lib-api? Unless I am not understanding what to mean not sure if this is a good idea.
| - `AIO_COMMERCE_BASE_URL` — the Commerce API base URL | ||
| - `AIO_COMMERCE_ENV` — the deployment type (`"saas"` or `"paas"`) | ||
|
|
||
| OpenWhisk injects all package parameters into every action's `params` object at invocation time, |
There was a problem hiding this comment.
I am afraid that this is coupling the implementation too much to OpenWhisk. Have you aligned this approach with the appbuilder team?
| `CommerceSystemConfig`, or `null` if either value is absent. The function is synchronous — no | ||
| async call is needed because the values are already present in `params`. | ||
|
|
||
| ### Changes to `commerce-app-management` |
There was a problem hiding this comment.
I believe we should not have commerce-app-management implementation details here. Maybe you can just describe broadly how a client of app-management api is impacted and the expected flows.
| * Returns the Commerce system configuration populated during association, | ||
| * or null if the app is not currently associated with a Commerce instance. | ||
| */ | ||
| export function getCommerceSystemConfig( |
There was a problem hiding this comment.
Not sure if the concept system config is too broad. I'd prefer something like getAssossociationDetails or getAssociatedCommerceInstance or similar
| The SDK is already passed `commerceBaseUrl` and `commerceEnv` at every association and | ||
| unassociation event — the data apps need is already flowing through the system. The missing piece | ||
| is persistence tied to the association lifecycle and a standard way for runtime actions to consume | ||
| it. |
There was a problem hiding this comment.
In order to implement some of the other requested features (e.g. helpers to publish events into the Event Ingress API) we'll also need to store data about the project/workspace we're associating with.
|
|
||
| **Goals:** | ||
|
|
||
| - Store the Commerce Base URL and deployment type (PaaS/SaaS) as OpenWhisk package parameters |
There was a problem hiding this comment.
I am not very much aware of how package parameters work, but given that we're applying this "save" under the context of the app-management package, does this work for actions that are set under other packages? The app-management package is only ours, but developers have their own.
| A runtime action can read these directly: | ||
|
|
||
| ```js | ||
| export async function main(params) { | ||
| const baseUrl = params.AIO_COMMERCE_BASE_URL; | ||
| const env = params.AIO_COMMERCE_ENV; | ||
| } |
There was a problem hiding this comment.
While having in params is somehow standard, it also forces you to move the params object everywhere you need this data. So if you are for example in a 4-level deep function call hierachy and you need this data only at that level, you will be forced to drill it down from the first layer down to the fourth.
Business configuration would allow retrieving this without coupling to params
| import { getCommerceSystemConfig } from "@adobe/aio-commerce-lib-app/runtime"; | ||
|
|
||
| export async function main(params) { | ||
| const config = getCommerceSystemConfig(params); |
There was a problem hiding this comment.
But then we would be coupling lib-app to lib-api? Unless I am not understanding what to mean not sure if this is a good idea.
| - **Association before this feature existed.** Apps associated before `POST /association` was | ||
| introduced have no stored parameters. `getCommerceSystemConfig` returns `null`. Apps must handle | ||
| this explicitly. |
There was a problem hiding this comment.
@asalloum5 This sounds to me an acceptable tradeoff. But was wondering if we could do something similar to the "Sync Commerce Scopes"? For those apps that do not have this data maybe a "Sync Association" or something? So that they do not need to: Unassociate + Associate
Might also be a "solution" (probably not the best) to the edge case that @oshmyheliuk mentioned about a Commerce Instance changing URLs.
|
|
||
| - The stored shape can be extended with `projectId` and `workspaceId` at association time, | ||
| providing data needed by other planned SDK features without requiring a new association step. | ||
| - `getAssociatedCommerceInstance` could become the foundation for a higher-level helper that |
There was a problem hiding this comment.
can you include this in the scope and change the fetch example to be using resolver function? otherwise code will be repeated to create the commerce api client from the association details which we can already encapsulate.
There was a problem hiding this comment.
Yes good point, as suggested I will update the spec to include getAssociatedCommerceClient in scope and replace the fetch example with the resolver function pattern. This will return a ready to use AdobeCommerceHttpClient built from the stored association data, so developers don't have to wire it up themselves. I will implement it in this PR.
| The association data is stored using the infrastructure already established in | ||
| **`aio-commerce-lib-config`** — specifically, the shared `getSharedState()` utility from | ||
| `aio-commerce-lib-config/source/utils/repository.ts`, which provides a lazy-initialized | ||
| Adobe I/O State client shared across the SDK. | ||
|
|
||
| A new module is added to `aio-commerce-lib-config` specifically for association data, | ||
| separate from the existing Business Configuration module. It uses the same `getSharedState()` | ||
| client but stores under a dedicated reserved key (`association`) rather than the | ||
| `configuration.{scopeCode}` keys used by Business Configuration. |
There was a problem hiding this comment.
I think it should be mentioned that the cache TTL should be specified for this configuration, as by default, TTL is 24 hours in aio lib state https://github.com/adobe/aio-lib-state/blob/main/doc/api.md#adobestateputoptions--object.
The max TTL of 1 year can be set.
There was a problem hiding this comment.
I will update accordingly. The default TTL of 24 hours is not appropriate here association data is long-lived and must not expire on its own. It should only be cleared explicitly on unassociation. We'll set it to the max TTL of 1 year, after 1 year without re-association the data will expire.
| The shape is designed to extend future fields (e.g. `projectId`, `workspaceId`) can be | ||
| added without a breaking change. | ||
|
|
||
| ### New `association` runtime action |
There was a problem hiding this comment.
Please mention require-adobe-auth: true in specs for those new actions, as they must be protected the same way as others.
There was a problem hiding this comment.
sure I will add auth to the action
| - **Reserved config key naming.** The exact key used to store the data in `aio-commerce-lib-config` | ||
| needs to be defined as a reserved SDK concern so that apps do not accidentally collide with it. |
There was a problem hiding this comment.
This contradicts line 93, as the key naming was already mentioned:
client but stores under a dedicated reserved key (`association`) rather than the
There was a problem hiding this comment.
sorry for the confusion, I will remove here
| **On association** — after the app is successfully registered with the Extension Manager, | ||
| the client calls `POST /` with the Commerce instance details. This step is best effort a failure does not roll back the registration. |
There was a problem hiding this comment.
If a POST/ request to the association endpoint fails, it may lead to a broken application if runtime actions depend on that configuration. I think we can't mark the app as successfully associated if the configuration wasn't saved. Probably the association should fail in such a case.
There was a problem hiding this comment.
Good point. However, I have a suggestion, please let me know if this approach sounds okay. Since the association with the Extension Manager and the config storage are separate concerns, we feel that a transient network error on the association endpoint should not block the entire association flow. Instead of failing the association completely, we plan to surface the failure in the App Management UI so developers are aware of it and can re-associate the app if needed to resolve the issue.
Description
Adds two new routes to the existing
installationRuntimeActioninaio-commerce-lib-app:POST /installation/association— storesAIO_COMMERCE_BASE_URLandAIO_COMMERCE_ENVas OpenWhisk package parameters when an app is associated with a Commerce instance
DELETE /installation/association— removes those parameters on unassociationAdds a new
@adobe/aio-commerce-lib-app/runtimeexport withgetCommerceSystemConfig(params),a synchronous helper that reads the stored config from any runtime action's params.
Related Issue
CEXT-6160
Motivation and Context
Every Commerce app that needs to call the Commerce API was reimplementing the same logic to
store and retrieve the Commerce Base URL. This also stored it at install time rather than
keeping it in sync with the association lifecycle.
OpenWhisk package parameters are injected automatically into every action's params at
invocation time — storing the config there means runtime actions receive the Commerce URL
and env with zero extra code, and it is always in sync with the current association state.
How Has This Been Tested?
POST /associationandDELETE /associationroute handlersgetCommerceSystemConfigcovering all cases (present, null, paas, saas)POST /associationandDELETE /associationroute handlersgetCommerceSystemConfigcovering all cases (present, null, paas, saas)POST /installation/associationreturns200 { baseUrl, env }after associationDELETE /installation/associationreturns204after unassociationdTypes of changes
Checklist